Thanks for the helpful review comments. In terms of what the inferred type of the result should be, maybe I'm fixing this in the wrong place. The original motivation was this report in Pylint; if we infer that typing.cast(A, x) has the same type as x then we won't fix this issue.

The way I look at it, typing.cast exists to provide information to make static checking better, so in an ideal world a linter could use the knowledge of the casted-to type to give better feedback. But does this mean that Pylint is the right place to apply this logic and not Astroid?

@timmartin I'm a bit hesitant with this one. Who makes sure A is truly the correct type? In an ideal world, x would be inferred correctly and we can just say that the result of cast equals x. Although I haven't looked at the issue in detail, it seems that there are two distinct issues:

1. Currently, the result of cast is always Uninferrable
2. wave.open might not be inferred correctly. This should be fixed separately by adding a brain module or writing a custom plugin.

One last thing, it's should be possible to use abstract classes for A just so the type checker is satisfied. However, that might prevent pylint from analyzing it further although it sometimes could.

The developer who adds the cast is responsible for checking that A is the right type. If they put the wrong annotation on, they'll get the wrong results from the type checker / linter. Garbage in, garbage out.

To me, the issue is that typing.cast is specifically designed to be used in the case where the correct type isn't / can't reasonably be inferred. It has no runtime semantics, it's purely a hint to the type checker. If x was being correctly inferred, there'd be no point in applying a cast.

I think you're right in theory that any problem that can be solved by a cast can be solved by sufficiently good type declarations / better inference. I think casting exists for the cases where that doesn't yet exist.

The issue I have with it is that cast is designed for type checking and not for inference. You could for example say that the type should be an abstract or protocol class. That will work perfectly fine for mypy, but won't offer any additional insides to us.

It might be an idea to add something like astroid.cast / pylint.cast that can be used explicitly for inference tips.

It might be an idea to add something like astroid.cast / pylint.cast that can be used explicitly for inference tips.

It would add work for pylint users, what is the meaningful difference between typing for inference and typing for type/mypy ? I think cast does the work and have the advantages of being official and widespread. Also as @timmartin said, garbage in, garbage out, if you mislead mypy or pylint you get the error you deserve 😄

@Pierre-Sassoulas The point I'm trying to make here is that with inference we can already do better then what type checkers can. So using cast doesn't add anything for us. In contrast it might even prevent us from offering better suggestions.

using cast doesn't add anything for us. In contrast it might even prevent us from offering better suggestions.

Well it's true that it might prevent us from offering better suggestions but only when the user made an explicit typing mistake. I think it add something for us if the inference fail. So maybe we should rely on cast if the inference failed and provide an option to be able to check cast ?

AIUI type checkers like mypy already do type inference, they can't rely on every variable having its type statically declared. Is there a class of inference that Astroid does that wouldn't be available in a sufficiently smart type checker?

I can certainly see how there will be cases in the real world where the cast may be needed for mypy and not for Pylint, say. That would make it annoying if adding the needed cast helped mypy but broke Pylint. On the other hand, there will be cases where Astroid isn't (yet) making the inference and it would be useful to have the option of a cast.

It should be easy enough to make it infer the type of cast(A, x) as the type of x if it's inferable, otherwise an instance of A. I think that would avoid making inference worse in any case, although it still might miss the opportunity to do (say) a downcast when the variable is known to be an instance of a parent class. Is that a good solution here?

TBH I don't have a strong opinion here, I just picked this one up because I thought it would be an easy way to get used to the codebase. If it's controversial, I'm happy to leave this to someone else.

@hippo91 do you have an opinion on this ?

Well that is not an easy question. Thanks all for giving your point of view. IMHO the best option here is to infer the result of the cast as 'A' only if the inference of 'x' failed. Otherwise I am afraid we lose precious informations as @cdce8p mentioned it. But I am afraid there is not a real clear solution for this.

Thanks for your input @hippo91!
I agree with your point and like the suggestion. So +1 from me

@Pierre-Sassoulas I would like to include it in 2.6.3 if that's fine with you. It will improve inference and thus help solve some issue.

@cdce8p Good catch. Is it reasonable to report uninferable in both cases? I can see how both cases could in principle be handled, but I don't know what infrastructure we already have for parsing type expression strings and working with typevars.

@timmartin Either that or raising UseInferenceDefault (which should ultimately also result in Uninferable).

I think my latest change deals with both cases. I'm not totally sure in the typevar case; in my testing it appears that the typevar just gets inferred to Uninferable, which we can handle easily. I don't know if this will suffice in general though, if we improve typevar inference presumably we could regress this functionality.

Just from a quick view, I fear we won't be able to easily find out if a type is a TypeVar or not. That unfortunately will bring us back to the point I made at the beginning: It's probably best to only infer cast as the variable itself even if it's Uninferable.

That's a shame. I'm a little puzzled though, because I was assuming that astroid would either successfully determine that the argument was a TypeVar, or else it would be uninferable. Are you saying that TypeVars are handled in a way that makes it impossible to distinguish a class from a typevar?

That's a shame. I'm a little puzzled though, because I was assuming that astroid would either successfully determine that the argument was a TypeVar, or else it would be uninferable. Are you saying that TypeVars are handled in a way that makes it impossible to distinguish a class from a typevar?

At the moment that is the case. Keep in mind though that this behavior is completely fine since astroid doesn't use type hints for inference tips. We just need to make sure they have some basic properties. There are probably some more cases that could cause issues: NewType, Protocol, Generic.

If that's the case then it's probably best to do as you suggest and just infer from the second argument.

Will astroid differentiate between classes and typevars in the future? It seems like it would be necessary to catch some errors e.g. that typevars can't be constructed.

Looking at the typevar code, is there any way to distinguish based on metaclass? It looks like typevars are given a custom metaclass; if we could only infer cast for classes with a default metaclass that might be a small win. I fiddled around for a while but I couldn't come up with anything that didn't feel like a kludge, though.

If that's the case then it's probably best to do as you suggest and just infer from the second argument.

I would probably prefer that.

Will astroid differentiate between classes and typevars in the future? It seems like it would be necessary to catch some errors e.g. that typevars can't be constructed.

Maybe, there are multiple issues though. First you need someone to add it, then to review it, and most important someone to maintain it. (1) might be possible, but the other two unlikely. As it currently is, I too struggle sometimes to understand a change completely. That's the cost of good, but really old legacy code. Regarding typing in particular, I'm happy that most major issues are solved now. It has taken us moths to do that at the start of the year. (Also a reason why I prefer the safe route here.)

Looking at the typevar code, is there any way to distinguish based on metaclass? It looks like typevars are given a custom metaclass; if we could only infer cast for classes with a default metaclass that might be a small win. I fiddled around for a while but I couldn't come up with anything that didn't feel like a kludge, though.

You could try next(node.args[0].infer()).metaclass(), but again I would probably suggest to leave it be even it might technically be possible. I found the two issues after only a short test run. Don't know what other will come up with once it is released. Usually you have to decide if the feature you are adding is worth the additional bugs it might create. I'm just really skeptical about that.

I know, that's probably not the answer you were hoping for but that's how I feel about it. I hope you can understand it.

I think my last change does what's required here. Should I change the tags on this PR to indicate that it's ready for re-review?

@timmartin there's a little "reload" button in the reviewers so you can ask for a new review that way. :)